Switch PianoKeyboard between Notated Pitch (effective pitch) and Playback Pitch (completed). #24784

rpatters1 · 2024-09-17T17:00:33Z

Resolves: #15594

Supercedes: #22991, which should be closed.

Added buttons in the PianoKeyboardPanelContextMenuModel class that communicate with the PianoKeyboardController class to change a boolean variable. The aim is to address a limitation in the current implementation of MuseScore's keyboard view, which struggles with differently-tuned instruments. When users click on a key, the corresponding note used to be placed on the score as notated pitch, while the equivalent standard pitch was highlighted on the keyboard UI, leading to potential confusion.

This PR also adds a change to Score::noteVal so that the correct pitch is entered into the score, based on the menu option selected.

I signed the CLA
The title of the PR describes the problem it addresses
Each commit's message describes its purpose and effects, and references the issue it resolves
If changes are extensive, there is a sequence of easily reviewable commits
The code in the PR follows the coding rules
There are no unnecessary changes
The code compiles and runs on my machine, preferably after each commit individually
I created a unit test or vtest to verify the changes I made (if applicable)

rpatters1 · 2024-09-17T19:59:43Z

This PR should be ready for review now. I moved the pianoKeyboardUseNotatedPitch getter and setter from INotationConfiguration to IEngravingConfiguration to allow accessing the property inside the Score class. Everything seems to be working.~~, though I am not currently set up to test configuration persistence across executions of MuseScore~~

EDIT: I tested without the -F option and it indeed preserves the setting across executions of MuseScore.

cbjeukendrup · 2024-09-17T21:42:29Z

src/engraving/dom/edit.cpp

@@ -1587,7 +1587,7 @@ NoteVal Score::noteVal(int pitch) const

    // if transposing, interpret MIDI pitch as representing desired written pitch
    // set pitch based on corresponding sounding pitch
-    if (!style().styleB(Sid::concertPitch)) {
+    if (!style().styleB(Sid::concertPitch) && configuration()->pianoKeyboardUseNotatedPitch().val) {


I'm not sure if we should do this check here. It might be preferable to move the setting back to notationConfiguration and retrieve its value outside the engraving module (since I'd rather consider it UI logic than business logic) and pass it as a bool parameter to Score::addMidiPitch and Score::noteVal.

I don't disagree with what you say at all, but I have trouble seeing any philosophical difference between the Display Concert Pitch setting and this new one as far as to whether they are business logic or UI. If we are going to move the new one, we should move the old one as well, and that may get into a rats' nest of refactoring. (You would know better than I whether it is a lot.)

Or maybe the noteVal function should not be part of Score. But that seems like gets into even more refactoring.

I would say that the difference is the scope of these settings. Sid::concertPitch is a score setting, and affects also how the score is typeset, i.e. really the content; pianoKeyboardUseNotatedPitch is a UI setting, independent of the score, affecting only the behaviour of the UI.

There are absolutely a lot more opportunities for refactoring in the Score class and things around note input... but let's save that for different PRs!

Hmm. I hope you will get to know me well enough that I sometimes when write out a counter argument, I then begin arguing against my own argument in my head and eventually convince myself that I was wrong. I think I get how Display Concert Pitch could be construed as business logic (perhaps because it controls engraving output) while there is no possibility of the input being so construed. So I will investigate your suggestion.

src/notation/view/pianokeyboard/pianokeyboardcontroller.h

cbjeukendrup · 2024-09-17T21:58:23Z

See also the recent discussion in #15594; the scope of the issue has been extended a little bit. Personally I prefer the two "use notated pitch" / "use sounding pitch" options over a single checkbox though, because with these options you really know what you are choosing from. I'll add that as a comment at #15594.

rpatters1 · 2024-09-17T23:06:45Z

The boolean (potentially) needs to be passed from three locations:

NotationMidiInput::doRealtimeAdvance
NotationMidiInput::addNoteToScore
NotationMidiInput::makeNote

I am passing the boolean in from the latter 2 but not the 1st (doRealtimeAdvance). The only one I have figured out how to test is addNoteToScore. Do you think they all 3 should pass in the boolean?

rpatters1 · 2024-09-17T23:07:48Z

I agree that the UI should be in Preferences. I'll tackle that on a different commit, though it can be on this PR if you wish. I would like to nail down the PR as it was.

cbjeukendrup · 2024-09-17T23:51:19Z

Hm, adding a bool parameter useNotatedPitch to Score::realtimeAdvance would also look a bit weird. One way to solve that would be to get rid of that method and move its content of that method to NotationMidiInput. Perhaps that's not a bad idea anyway.

Re. how to test: when you right-click the note input button in the toolbar, you get a menu where you can choose "realtime" note input mode. I've never studied how this works, but maybe the MuseScore 3 (!) handbook is useful: https://musescore.org/en/handbook/3/note-input-modes#realtime-auto

Making the preferences changes in separate commits to this PR seems good to me.

rpatters1 · 2024-09-18T00:22:28Z

Given that Score is business logic, I provided a name for the boolean that makes no reference to the piano keyboard: allowTransposition. Does adding that parameter to Score::realtimeAdvance seem weird? Given how it works, it doesn't seem weird to me, but if you want me to I'll look at moving. It gets called exactly once by NotationMidiInput::doRealtimeAdvance.

Perhaps allowTransposition describes more precisely what the business logic is doing than useNotatedPitch would. Basically, if Display Concert Pitch is unchecked, the business logic transposes the note(s). This new parameter says only do that when it is allowed.

rpatters1 · 2024-09-19T00:12:17Z

I have removed the menu items from the piano keyboard and added them to the playback settings menu, as shown.

I went ahead and changed the text to simple Anglo Saxon words instead of the fancy French words that we were using before. This is consistent with prior art. (I'm assuming menu item text is not copyrighted. If so, we can change it.)

rpatters1 · 2024-09-19T00:29:47Z

Assuming the checks pass, it looks ready for review to me. (I've reviewed the cumulative file changes and they now all look as I intended them to.) The names have all been changed to match the new menu items and location of the menu.

…EngravingConfiguration to make it accessible in Score class.

…core, plus other codereview fixes.

…ming)

…, arghh)

src/notation/view/pianokeyboard/pianokeyboardcontroller.cpp

src/playback/internal/playbackcontroller.cpp

rpatters1 · 2024-10-02T18:07:12Z

excellent suggestions both. update is posted

src/playback/internal/playbackcontroller.cpp

cbjeukendrup

Apart from that last thing, looks good to me!

rpatters1 · 2024-10-02T18:43:54Z

they say naming is the hardest thing in programming. fixed it.

Jojo-Schmitz · 2024-10-02T19:27:10Z

they say naming is the hardest thing in programming. fixed it.

There are only two hard things in Computer Science: cache invalidation and naming ... things.

-- Phil Karlton

There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.

-- Leon Bambrick

There are only two hard problems in distributed systems: 2. Exactly-once delivery 1. Guaranteed order of messages 2. Exactly-once delivery

-- Mathias Verraes

there's two hard problems in computer science: we only have one joke and it's not funny.

-- Phillip Scott Bowden

There are so many variations on the “there are only two hard problems in computer programming...” joke that I’m starting to suspect that programming isn’t actually very easy.

-- Nat Pryce

zacjansheski · 2024-10-02T20:38:38Z

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#15594 FIXED

avvvvve · 2024-10-03T21:43:39Z

Functionally this looks good, and the naming and placement seem sensible to me! I've asked for @bkunda's signoff on the latter and I have one suggestion for a different icon we could use (the tuning fork that's used for "Concert pitch"), but we can save that for a part 2.

cbjeukendrup · 2024-10-04T19:05:07Z

@rpatters1 I'll merge this PR right now. After some discussion today, we decided that we might want to make some further tweaks how and where this option is presented; see #25033. The exact design is still in progress, but we'll ping you when it's ready, in case you would like to implement that part as well. Thanks!

rpatters1 · 2024-10-04T19:34:37Z

I'm up for doing the implementation. If we are using a new icon, I'd prefer someone else design it. (Graphic design is not where my talents lie.😅)

rpatters1 mentioned this pull request Sep 17, 2024

Added buttons for PianoKeyboard to switch between Notated Pitch (effective pitch) and Playback Pitch. #22991

Closed

7 tasks

cbjeukendrup reviewed Sep 17, 2024

View reviewed changes

src/notation/view/pianokeyboard/pianokeyboardcontroller.h Outdated Show resolved Hide resolved

cbjeukendrup mentioned this pull request Sep 17, 2024

Feature request: Toggle for behaviour of on-screen/MIDI keyboard regarding transposing instruments #15594

Closed

rpatters1 force-pushed the rpatters1/keyboard-switch-concert-transposed branch from fd5ecbd to a2edd6f Compare September 19, 2024 00:04

rpatters1 force-pushed the rpatters1/keyboard-switch-concert-transposed branch from 552f0d2 to dfcb72d Compare September 27, 2024 11:50

rpatters1 added 12 commits October 2, 2024 08:06

Changes to revive and complete PR musescore#22991.

9132db0

no message

d5ca74a

changes for all midi inputs

9711ea2

Move property getter and setter for pianoKeyboardUseNotatedPitch to I…

fc698e7

…EngravingConfiguration to make it accessible in Score class.

uncrustification

c244f27

remove unneeded stub functions

867ca44

reverted to using INotationConfiguration with a boolean passed into S…

03e217a

…core, plus other codereview fixes.

move menu to playback bar and use terse menu text

47d2f72

make function name consistent with UI

8c8c62e

clean up cruft

144de39

one more name change (they say naming is the hardest thing in program…

ab64440

…ming)

stub module did not get name changes (because it's not in any project…

d44b99e

…, arghh)

rpatters1 force-pushed the rpatters1/keyboard-switch-concert-transposed branch from dfcb72d to d44b99e Compare October 2, 2024 13:22

cbjeukendrup reviewed Oct 2, 2024

View reviewed changes

src/notation/view/pianokeyboard/pianokeyboardcontroller.cpp Outdated Show resolved Hide resolved

src/playback/internal/playbackcontroller.cpp Outdated Show resolved Hide resolved

code review items

1dc1d35

cbjeukendrup reviewed Oct 2, 2024

View reviewed changes

src/playback/internal/playbackcontroller.cpp Outdated Show resolved Hide resolved

cbjeukendrup approved these changes Oct 2, 2024

View reviewed changes

cbjeukendrup assigned zacjansheski Oct 2, 2024

cbjeukendrup requested a review from avvvvve October 2, 2024 18:29

naming is the hardest thing

6a0159d

avvvvve approved these changes Oct 3, 2024

View reviewed changes

avvvvve mentioned this pull request Oct 3, 2024

Written/sounding pitch switch, part 2 #25033

Open

cbjeukendrup merged commit 4662ac8 into musescore:master Oct 4, 2024
11 checks passed

rpatters1 deleted the rpatters1/keyboard-switch-concert-transposed branch October 19, 2024 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch PianoKeyboard between Notated Pitch (effective pitch) and Playback Pitch (completed). #24784

Switch PianoKeyboard between Notated Pitch (effective pitch) and Playback Pitch (completed). #24784

rpatters1 commented Sep 17, 2024

rpatters1 commented Sep 17, 2024 •

edited

Loading

cbjeukendrup Sep 17, 2024

rpatters1 Sep 17, 2024

cbjeukendrup Sep 17, 2024

rpatters1 Sep 17, 2024

cbjeukendrup commented Sep 17, 2024

rpatters1 commented Sep 17, 2024

rpatters1 commented Sep 17, 2024

cbjeukendrup commented Sep 17, 2024 •

edited

Loading

rpatters1 commented Sep 18, 2024

rpatters1 commented Sep 19, 2024

rpatters1 commented Sep 19, 2024

rpatters1 commented Oct 2, 2024

cbjeukendrup left a comment

rpatters1 commented Oct 2, 2024

Jojo-Schmitz commented Oct 2, 2024 •

edited

Loading

zacjansheski commented Oct 2, 2024

avvvvve commented Oct 3, 2024

cbjeukendrup commented Oct 4, 2024

rpatters1 commented Oct 4, 2024

Switch PianoKeyboard between Notated Pitch (effective pitch) and Playback Pitch (completed). #24784

Switch PianoKeyboard between Notated Pitch (effective pitch) and Playback Pitch (completed). #24784

Conversation

rpatters1 commented Sep 17, 2024

rpatters1 commented Sep 17, 2024 • edited Loading

cbjeukendrup Sep 17, 2024

Choose a reason for hiding this comment

rpatters1 Sep 17, 2024

Choose a reason for hiding this comment

cbjeukendrup Sep 17, 2024

Choose a reason for hiding this comment

rpatters1 Sep 17, 2024

Choose a reason for hiding this comment

cbjeukendrup commented Sep 17, 2024

rpatters1 commented Sep 17, 2024

rpatters1 commented Sep 17, 2024

cbjeukendrup commented Sep 17, 2024 • edited Loading

rpatters1 commented Sep 18, 2024

rpatters1 commented Sep 19, 2024

rpatters1 commented Sep 19, 2024

rpatters1 commented Oct 2, 2024

cbjeukendrup left a comment

Choose a reason for hiding this comment

rpatters1 commented Oct 2, 2024

Jojo-Schmitz commented Oct 2, 2024 • edited Loading

zacjansheski commented Oct 2, 2024

avvvvve commented Oct 3, 2024

cbjeukendrup commented Oct 4, 2024

rpatters1 commented Oct 4, 2024

rpatters1 commented Sep 17, 2024 •

edited

Loading

cbjeukendrup commented Sep 17, 2024 •

edited

Loading

Jojo-Schmitz commented Oct 2, 2024 •

edited

Loading